fix(core/txpool): keep local tracker consistent with gas price floor - #2541
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
gzliudan
requested review from
AnilChinchawale,
anunay-xin,
benjamin202410,
liam-lai and
wanwiset25
and
a balanced review from Copilot
August 25, 2026 22:08
There was a problem hiding this comment.
Pull request overview
Fixes local transaction tracking so permanently rejected or superseded transactions are removed from memory and disk.
Changes:
- Classifies resubmission errors and untracks permanent rejections.
- Removes superseded same-nonce transactions.
- Rotates journals after removals and adds regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
core/txpool/locals/tx_tracker.go |
Updates resubmission, untracking, and journal rotation logic. |
core/txpool/locals/tx_tracker_test.go |
Tests rejection, replacement, and journal persistence behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
gzliudan
force-pushed
the
untrack-lower-price-tx
branch
from
August 25, 2026 23:39
d34ee67 to
69d8ff1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
core/txpool/locals/tx_tracker_test.go:165
- Handle the key-generation error instead of passing a potentially nil key to
SignTx; the repository's Go error-handling convention does not allow ignored errors.
otherKey, _ := crypto.GenerateKey()
benjamin202410
approved these changes
Aug 27, 2026
benjamin202410
approved these changes
Aug 27, 2026
gzliudan
force-pushed
the
untrack-lower-price-tx
branch
from
August 27, 2026 07:28
69d8ff1 to
caf9b92
Compare
wanwiset25
approved these changes
Aug 30, 2026
gzliudan
force-pushed
the
untrack-lower-price-tx
branch
11 times, most recently
from
August 30, 2026 22:49
1c1fb44 to
dd015f2
Compare
anunay-xin
approved these changes
Aug 31, 2026
gzliudan
force-pushed
the
untrack-lower-price-tx
branch
from
August 31, 2026 10:14
dd015f2 to
c5373cf
Compare
gzliudan
force-pushed
the
untrack-lower-price-tx
branch
from
August 31, 2026 16:57
c5373cf to
f9b404a
Compare
gzliudan
changed the title
gzliudan
force-pushed
the
untrack-lower-price-tx
branch
3 times, most recently
from
August 31, 2026 22:48
a9ba231 to
247dfcd
Compare
…aces TrackAll only ever added to the tracked set, while the per-nonce SortedMap silently overwrote the entry a replacement displaced. The replaced transaction was therefore never returned by Forward again, stayed tracked forever, was rewritten into the journal on every rotation and could win the nonce on the next load, resurrecting a transaction the user had already replaced, because rotation writes the tracked set in map order through a non-stable sort. Decide which of the two supersedes the other the way the pool decides it. While the pool holds one of them it is authoritative: it occupies a nonce with at most one transaction, and AddLocal tracks a local transaction only after Add has released the subpool lock, so two concurrent submissions can be accepted in one order and reach TrackAll in the other. When the pool holds neither -- two submissions it has since discarded, or a journal an older version wrote -- fall back to the substitution rules legacypool applies in list.Add: a special transaction always claims its nonce, a regular one must not evict a pending special one, and otherwise a replacement has to beat the transaction it replaces on both fee cap and tip. The price bump is deliberately not repeated here: it is pool policy, and a transaction that beats the old one but misses the bump behaves exactly as before, so no case gets worse. Load now converges a journal written by an older version on the replacement rather than on whichever entry the rotation happened to sort last, so the entry later in the file no longer decides the nonce. That is a behaviour change for journals left behind by an older version: the replacement survives now even when it was written first. Equally priced transactions are not a substitution either, so the one already tracked keeps the nonce where the previous behaviour let the later one take it. Build the test environment from an explicit chain config so tests can pin the gas schedule instead of sharing the package level genesis, and price the shared replacement pair to clear the pool's price bump, which is what makes it a substitution rather than two transactions the pool would reject. Cover both orders in which a replacement can reach the tracker, the concurrent interleaving where the original is added first and tracked last, the fallback for two transactions the pool holds neither of, the special transaction rules in both directions, the tie between equally priced transactions, and a dearer tracked transaction losing to the one the pool accepted.
…the gas price floor A gas schedule fork raises the floor above transactions that were admitted under the previous tier, and the pool sweeps them out. The local tracker kept resubmitting them every minute: recheck ignored the result of pool.Add, and the transactions never went stale because their nonce never advanced, so they stayed tracked and journalled forever. Hold back any tracked transaction priced below the current floor, resolved for the block pending on top of the head exactly as admission validation resolves it. They stay tracked, so a reorg or a set-head rollback that lowers the floor picks them up again on the next recheck: the pool does not bring back what it swept, so the tracker is the only thing that can. Special transactions stay exempt, as they are during admission. Resolve the floor through a new TxPool.MinGasPrice, which goes through the same pendingBlockNumber helper as admission validation and the sweep, so the tracker and the pool cannot price the same transaction at different heights. Report the held back transactions through txpool/local/belowfloor, since they are still counted by txpool/local. Pin that ErrUnderMinGasPrice is not a temporary reject: the floor only rises as the chain advances, so retrying cannot help and AddLocal must not track the transaction, but the error must not be used to drop a tracked one either. Cover the floor with tests that rewind the head with SetHead across every tier boundary and cover a head without a block number, the hold-back at exactly the floor and one wei below it, resumption once the floor drops, and the exemption special transactions keep.
gzliudan
force-pushed
the
untrack-lower-price-tx
branch
from
August 31, 2026 23:51
247dfcd to
8a16b0f
Compare
gzliudan
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two fixes to keep the local transaction tracker aligned with the gas price floor and with the pool's nonce ownership. Hold back local resubmits priced below the current gas price floor, so a gas schedule fork that raises the floor no longer makes the tracker re-submit transactions that can only fail with
ErrUnderMinGasPrice; held-back transactions stay tracked and are picked up again automatically if the floor drops (reorg / rollback past the fork). Drop a tracked transaction that is superseded at its nonce by another tracked transaction inTrackAll, instead of leaving both in the journal forever (which could resurrect a replaced transaction on the next journal load).Motivation & Context
After a gas tier fork the floor can rise above transactions admitted under the previous tier. The tracker's periodic
recheckwould re-submit them every cycle, only to be rejected withErrUnderMinGasPrice, andErrUnderMinGasPricewas previously treated as a temporary rejection, which is wrong: the floor only rises, so retrying cannot help while the tx stays where it is, and dropping a tracked tx is wrong because a rollback past the fork would make it admissible again. The tracker previously put every tracked tx intoallwithout considering that two tracked transactions can share a nonce (concurrent submissions, or a journal written by an older version holding both a tx and its replacement), and the silent overwrite inSortedMap.Putleft the superseded tx stranded inall/journal, where it could win the nonce on reload.Changes
core/txpool/validation.go: addpendingBlockNumberhelper; admission validation now uses it (no behavior change).core/txpool/txpool.go: addTxPool.MinGasPrice()returning the floor resolved at the pending-block height.core/txpool/locals/tx_tracker.go:TrackAlldrops the tracked tx superseded at a nonce, deciding the winner the same way the pool does (which tx the pool holds, special-tx priority, then fee-cap/tip comparison);recheckskips non-special tracked txs below the floor (counted in the newtxpool/local/belowfloorgauge) and returns the rest for resubmission;IsTemporaryReject(errors.godoc) explicitly excludesErrUnderMinGasPrice. Tests:tx_tracker_test.go,txpool_test.go,txpool_local_test.go,errors_test.gocover both behaviors.Testing
New/updated unit tests in
core/txpoolandcore/txpool/locals. Runmake quick-test/make test, orgo test ./core/txpool/... ./core/txpool/locals/....Related Issues
None.
Risk & Impact
Local (tracked) transactions only; the public pool admission path is unchanged except the floor is now resolved through a shared helper. Behavior change: tracked transactions priced out by a fork are no longer re-submitted every minute and are no longer eligible to be dropped as stale; they persist until admissible again, which avoids log/metric spam and incorrect drops. Journal convergence: journals written by older versions holding a tx and its replacement now converge to the replacement on load.
Notes for Reviewers
TxPool.MinGasPriceresolves the floor against the chain head, while admission resolves it against the subpool head; the two can drift briefly while a head event is in flight, but this is self-correcting on the nextrecheck(period far longer than the drift window) — see the doc comment. The nonce-supersession decision inTrackAlldeliberately does not repeat the price-bump policy, to avoid making any case worse than before.TODO
None.